Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Incorrect V for EIP712 signature #152

Merged
merged 2 commits into from
Aug 24, 2022
Merged

Incorrect V for EIP712 signature #152

merged 2 commits into from
Aug 24, 2022

Conversation

lambertkevin
Copy link
Contributor

As described by #134 and #151, the V parsed by Metamask for a signTypedData is incorrect. The signature is not expected to be composed with the parity/recovery but with the classic 27 or 28.

This PR fixes this and updates the CI tests.

Thanks 🙏

@lambertkevin lambertkevin requested a review from a team as a code owner July 26, 2022 15:36
digiwand
digiwand reviewed Aug 17, 2022
View reviewed changes
index.js Show resolved Hide resolved
@lambertkevin lambertkevin requested a review from digiwand August 22, 2022 14:48
@FrederikBolding
Copy link
Member

@digiwand @lambertkevin Could we test this implementation against https://github.com/MetaMask/eth-sig-util to prevent issues like this in the future?

It may be out of scope for this PR.

@lambertkevin
Copy link
Contributor Author

lambertkevin commented Aug 22, 2022 •
edited
Loading

@digiwand @lambertkevin Could we test this implementation against https://github.com/MetaMask/eth-sig-util to prevent issues like this in the future?

It may be out of scope for this PR.

Hi @FrederikBolding !

Well the problem is that to my knowledge, @metamask/eth-sig-utils recoverPersonalSignature & recoverTypedSignature and ethers.js recoverAddress are all ignoring the value of v accepting 0 and 1 as a valid v because of EIP-1559, which means it doesn't matter what lib you're going to test the implementation against, you won't catch a v issue like this one. 🤔

And that's actually the reason why the issue is noticed only now that people start using ECDSA verification onchain with EIP-712 messages, because contrary to those libs, ecrecover is returning 0x0 with a v = 0 | 1. And since most of EIP-191 messages were only tested in frontends, no one had really catched the issue before 🤷‍♂️

digiwand
digiwand approved these changes Aug 22, 2022
View reviewed changes
Copy link

@digiwand digiwand left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the code with macOS x Chrome x Nano X using the test-dapp. LGTM! Thanks @lambertkevin

will hold on merging for @FrederikBolding, or another dev, to review

@digiwand digiwand merged commit dec4772 into MetaMask:main Aug 24, 2022
@lambertkevin lambertkevin deleted the bugfix/fix-v-in-sign-typed-data branch August 24, 2022 15:51
@xgambitox xgambitox mentioned this pull request Sep 7, 2022
Merged
@mcmire mcmire mentioned this pull request Jan 18, 2023
Closed
@cryptoKevinL
Copy link

I am seeing this issue as well, where signatures are different by decimal 27 between Ledger Live and Metamask+Ledger

@plasmacorral plasmacorral mentioned this pull request Jun 16, 2023
Closed
@plasmacorral plasmacorral mentioned this pull request Jul 12, 2023
Closed
@danjm danjm mentioned this pull request Nov 30, 2023
Closed
@plasmacorral plasmacorral mentioned this pull request Jan 6, 2024
Closed
@plasmacorral plasmacorral mentioned this pull request Feb 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@digiwand digiwand digiwand approved these changes

@jpuri jpuri jpuri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lambertkevin @FrederikBolding @cryptoKevinL @jpuri @digiwand